Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Overhaul client to better conform to OAuth2 spec #6

Merged
merged 12 commits into from Dec 28, 2012
Merged

Overhaul client to better conform to OAuth2 spec #6

merged 12 commits into from Dec 28, 2012

Conversation

conradev
Copy link
Contributor

I made a slew of changes to the client, ones that I hope push it in the right direction.

Changes Made

  • I updated the library to use ARC.
  • I removed AFOAuthAccount. I made this decision because an account abstraction only makes sense for the password grant type. The password grant type is the only grant type with any sort of account identifier (i.e. username) in the request or response. The response from the token endpoint only returns information about the token, and the best representation for this is a 'credential' object. Thus, all authentication success handlers are now given a AFOAuthCredential object. The removal of the account class also prompted me to rename AFOauthAccountCredential to AFOAuthCredential.
  • I transformed the client_id and client_secret values into readonly properties on the client object. This removes the need to pass these values as arguments repeatedly, when there is no need to. They are fixed and unchanging values. This mimics the behavior in AFOAuth1Client, where the key and secret properties are treated in much the same manner.
  • I removed the check for token expiration in the success callback of the OAuth token requests. An expired credential would never be returned from a request to the token endpoint. In addition to this, the success callback also now checks for the error field and calls the failure callback if appropriate.
  • I removed the secret property from the credential object, because it is intrinsic to the client, and not the credential. I added the tokenType property because it provides critical information on how the access token should be used.
  • I added the header Accept: application/json to each OAuth request, because all responses should be formatted in JSON, according to the spec. For this reason, AFJSONRequestOperation is also registered as an HTTP operation class by default.
  • Support for the authorization_code and client_credentials grant types was added.
  • Simple keychain persistence was added to AFOAuthCredential.
  • This encompasses the features in Fix some errors in README code examples. #2, Adds support for "password" grant type and the setting of the token value format. #3 (?), Update grant_type and Authorization headers for OAuth spec compliance. #4, Does not support authorization code grant type #5 and Support storing access token in keychain #7

Conrad Kramer added 6 commits November 27, 2012 00:11
This commit also includes a bunch of other changes.

The only non-functional change made to the library is that the code has
been updated to support ARC.

The first change made to the library was to remove AFOAuthAccount. This
decision was made because an account abstraction only makes sense for
the `password` grant type. The `password` grant type is the only grant
type with any sort of account identifier in the request or response. The
response from the token endpoint only returns information about the
token, and the best representation for this is a 'credential' object.
Thus, all authentication success handlers are now given a
`AFOAuthCredential` object. The removal of the account class also
prompted `AFOauthAccountCredential` to be renamed to `AFOAuthCredential`

The second change made to the library was to make the `client_id` and
`client_secret` values into readonly properties on the client object.
This removes the need to pass these values as parameters repeatedly,
when there is no need to. They are fixed and unchanging values. This
mimics the behavior in `AFOAuth1Client`, where the `key` and `secret`
properties were treated in much the same manner.

The third change to the library was to remove the check for token
expiration in the success callback for sending an OAuth token request.
While checking for an expired credential and renewing it is a good idea,
an expired credential would never be returned from a request to the
token endpoint. In addition to this, the success callback also now
checks for an `error` field and calls the `failure` callback if it
exists.

The fourth change made to the library was to the credential object. The
`secret` property was removed from the object, because it is intrinsic
to the client, not the credential. The `tokenType` property was added
because it provides critical information on how the access token should
be used.

The fifth change was to add the header `Accept: application/json` to
each OAuth request, because all responses should be formatted in JSON,
according to the spec. For this reason, the `AFJSONRequestOperation` is
also registered as an HTTP operation class. The `parameterEncoding`
property is left untouched, at the default value of
`AFFormURLParameterEncoding`.

Lastly, support for the `authorization_code` grant type was added, as
per the OAuth spec.
@kylef
Copy link

kylef commented Nov 28, 2012

Nice work, good timing too. Switching to this branch because I need scopes.

One issue: The operation variable on line 152 (and again on 171) shadows its the other operation.

AFHTTPRequestOperation *operation = [self HTTPRequestOperationWithRequest:mutableRequest success:^(AFHTTPRequestOperation *operation, id responseObject) {

This prevents compilation with warnings are errors settings.

@conradev
Copy link
Contributor Author

Thank you @kylef for pointing that out! I don't know why I didn't see it, even though I have -Wall and -Wextra set. Fixed.

Uses NSCoding to encode the credentials
@conradev
Copy link
Contributor Author

conradev commented Dec 3, 2012

I added basic keychain persistence, based on NSCoding, to store AFOAuthCredential objects in the keychain. The feature is disabled unless _SECURITY_SECITEM_H_ is defined, much like reachability and MIME type detection in AFHTTPClient.

This fixes #7

Conrad Kramer added 3 commits December 3, 2012 16:37
From Section 6 of RFC 6749:

"The authorization server MAY issue a new refresh token, in which case
the client MUST discard the old refresh token and replace it with the
new refresh token."

The client was not falling back on the old refresh token in the new
credential object, if none was specified in the response.
@ay8s
Copy link

ay8s commented Dec 7, 2012

Great work @conradev. Definitely useful additions, making use of the authorization_code implementation which works perfectly. Would love to see this merged in.

@dennisreimann
Copy link

Is there an official statement about the changes? I'd also like to see them pulled in, but this repo hasn't been updated for months now...

@conradev
Copy link
Contributor Author

Hopefully @mattt will get around to looking at this.

For now, I will maintain my fork.

@kylef
Copy link

kylef commented Dec 15, 2012

Here's a podspec if anyone else needs it for this fork. https://gist.github.com/raw/c367b5aa2864941c8f5e/71b23998c8616acf7c955e64405bd253a7c26c05/gistfile1.txt

@conradev
Copy link
Contributor Author

As per AFNetworking 1.1, the class constructor now returns instancetype (09f2f5c)

Funny observation: this repository has the third most number of stars, and is the second most out of date of the AFNetworking repositories

@mattt mattt merged commit 09f2f5c into AFNetworking:master Dec 28, 2012
@mattt
Copy link
Contributor

mattt commented Dec 28, 2012

An apology is in order to everyone who was inconvenienced by my mismanagement of this repository. It was irresponsible of me to let this languish for so long, and I humbly apologize for all of the hassle anyone faced in trying to use this in the meantime.

@conradev, your work on this pull request is superb. Thank you for your hard work on this. I've merged this in and tagged it as the 0.1.0 release.

@ay8s
Copy link

ay8s commented Dec 28, 2012

No apology needed mattt. Great work as ever 👍

Also, thanks @conradev!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants